fix: write gallery persona manifests as UTF-8 - #251
Conversation
The gallery install path hashes the manifest markdown as UTF-8, and
personas/manifest.py reads it back as UTF-8, but the temp file written
between them used Path.write_text() with no encoding. That falls back to the
locale encoding. On Linux and macOS the locale is UTF-8 and all three agree.
On Windows it is cp1252, and gallery copy routinely contains characters
cp1252 cannot represent.
Two distinct failures follow:
* CJK and emoji raise UnicodeEncodeError during the write, so the install
returns {"ok": false, "error": "'charmap' codec can't encode characters
in position ..."}.
* An em-dash or a curly quote encodes fine as a single cp1252 byte, so the
write succeeds and load_manifest_file() then fails reading it back as
UTF-8. Persona copy almost always contains these.
Pass encoding="utf-8" on the write, and make the .encode() above it explicit
so all three sites visibly agree on one encoding.
rajpratham1
left a comment
There was a problem hiding this comment.
This pull request improves cross-platform reliability for gallery persona installation by explicitly using UTF-8 when hashing and writing temporary manifest files. This keeps the encoding consistent with the manifest loader and avoids failures on systems whose default locale encoding is not UTF-8, particularly for manifests containing CJK characters, emoji, curly quotes, or em dashes. The added regression test verifies successful installation of a manifest containing diverse Unicode characters, while the existing hash mismatch test continues to validate integrity checking. Based on the visible changes, the implementation is focused, addresses the reported encoding issue cleanly, and no blocking issues are apparent.
Problem
The gallery persona install path uses three encodings for one piece of text, and only two of
them agree.
Path.write_text()with noencodingfalls back tolocale.getpreferredencoding(). On Linuxand macOS that is UTF-8, so all three agree and CI passes. On Windows it is cp1252.
Impact
Two different failures, depending on which characters the gallery copy contains.
Characters cp1252 cannot represent (CJK, emoji) raise
UnicodeEncodeErrorduring the write. Theblanket
except Exceptiona few lines below turns it into a response the user sees as:{"ok": false, "error": "'charmap' codec can't encode characters in position 39-40: character maps to <undefined>"}Characters cp1252 can represent but UTF-8 encodes differently are worse, because the write
succeeds. An em-dash becomes the single byte
0x97, a right curly quote becomes0x92. The filelands on disk looking fine, and then
load_manifest_file()fails reading it back:Persona marketing copy almost always contains em-dashes and curly quotes, so in practice gallery
install is broken on Windows for anything but a pure-ASCII manifest.
registry._snapshot()copies the manifest withshutil.copy2, a byte copy, so bad bytes alsopropagate into the installed snapshot under
installed_dir/<id>/manifest.md.Fix
Pass
encoding="utf-8"on the write. I also made the.encode()above it explicit rather thanrelying on its UTF-8 default, since the whole point is that these sites must visibly agree.
Tests
Added
test_gallery_install_handles_non_ascii_manifesttotests/test_cloud_server.py, reusingthe existing
_stub_galleryharness. The manifest carries an em-dash, curly quotes, CJK and anemoji, and the test asserts
nameandtaglinesurvive the round trip rather than just checkingthe call did not raise. Because it reads those back through
list_all(), it also covers theshutil.copy2snapshot, not only the initial write.Before the fix, on Windows:
Verification
Linux, Python 3.12, matching CI, run in Docker: 890 passed, 1 skipped, 0 failed (full suite).
Windows 11, Python 3.11.9: the new test goes from failing to passing. Full suite moves from
9 failed, 880 passedto8 failed, 882 passed.To be precise about that delta, since it is +2 rather than +1: one is the new test. The other is
test_ui_refresh_e2e, which is flaky on Windows and happened to pass on this run. This change didnot fix it, and #210 already addresses that flakiness.
The 8 remaining Windows failures are all pre-existing and unrelated to this change: four in
test_slack_relayplus one intest_github_installs(deterministic 2s timeouts),test_workspace_command_trust_controls_live_engine(WinError 32on a directory rename), and thetwo
user-only filepermission tests that #250 covers.Note
Found by running the suite on Windows. CI is
ubuntu-latestonly, where the locale is UTF-8, sothis class of defect cannot currently surface upstream.